Conversation
kocsismate
force-pushed
the
uri
branch
7 times, most recently
from
0667c3e to
c074b2a
Compare
kocsismate
force-pushed
the
uri
branch
4 times, most recently
from
3ec979f to
893cc0e
Compare
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
First pass at comments. Didn't yet look at all files, but this looks like a good start. Will take another look after you reviewed my comments to avoid looking at all files twice in case they change.
| <refsect1 role="seealso"> | ||
| &reftitle.seealso; | ||
| <simplelist> | ||
| <member><methodname>Uri\WhatWg\Url::getPassword</methodname></member> |
There was a problem hiding this comment.
Here it might make sense to also link to Uri\WhatWg\Url::withUsername (second position).
| <member><methodname>Uri\WhatWg\Url::getPassword</methodname></member> | |
| <member><methodname>Uri\WhatWg\Url::getPassword</methodname></member> | |
| <member><methodname>Uri\WhatWg\Url::withUsername</methodname></member> |
There was a problem hiding this comment.
I think doing so would make more sense for Uri because password is just a pseudo component together with username. For WHATWG URL, they are completely separate components with not much more connection than e.g. the host and port. I'm not against adding it though, so if you have a strong opinion, then I'm fine with it.
There was a problem hiding this comment.
For WHATWG URL, they are completely separate components with not much more connection than e.g. the host and port.
I feel username and password logically belong together (e.g. because setting a password will also add an empty username) and thus I would cross-link them.
Co-authored-by: Jordi Kroon <jkroon@onyourmarks.agency>
|
I've just aded the documentation for the two ext/uri enums. Could you please have another look sometime soon? :) |
TimWolla
left a comment
TimWolla
left a comment
There was a problem hiding this comment.
Okay, gone through all files now. Skimmed some of them. If this is resolved, the PR should be good enough for merging and I'll then double-check the rendered version and send PRs 😄
| <refsect1 role="seealso"> | ||
| &reftitle.seealso; | ||
| <simplelist> | ||
| <member><methodname>Uri\WhatWg\Url::getPassword</methodname></member> |
There was a problem hiding this comment.
For WHATWG URL, they are completely separate components with not much more connection than e.g. the host and port.
I feel username and password logically belong together (e.g. because setting a password will also add an empty username) and thus I would cross-link them.
| <refsect1 role="seealso"> | ||
| &reftitle.seealso; | ||
| <simplelist> | ||
| <member><methodname>Uri\Rfc3986\Uri::getRawPassword</methodname></member> |
There was a problem hiding this comment.
Should this link to the userinfo methods?
2 participants
Another attempt to document ext/uri after #5060
Some of my sentences are very clunky, so I am happy to receive suggestions.